Skip to content

[skip changelog] Use appropriate name for field that stores library folder name #1900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 4, 2022
Merged

[skip changelog] Use appropriate name for field that stores library folder name #1900

merged 1 commit into from
Oct 4, 2022

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Oct 2, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

Each Arduino library has a name, which is used as its sole identifier by the user in arduino-cli lib commands, and by Arduino CLI when referring to the library in messages displayed to the user.

The name is defined by:

The name is resolved when loading the library and stored in the Name field of the github.com/arduino/arduino-cli/arduino/libraries.Library struct.

The name of the library's installation folder is used by Arduino CLI in several other ways, most notably for determining "folder name priority" for use in library dependency resolution. For this reason, the folder name is also stored in the struct when loading the library.

Arduino CLI and arduino-builder have been plagued by problems caused by the inappropriate use of this folder name as the identifier for the library instead of the sole correct identifier (which is only the folder name in the case of "1.0 format" libraries (#932). The design of the github.com/arduino/arduino-cli/arduino/libraries.Library struct may have been a contributing factor in those bugs, since at the time of their occurrence the folder name was stored in the Name field, the metadata-defined name in a RealName. In addition to the fact that no one field could be used as a source of the name in all cases, I suspect the ambiguous field names themselves caused confusion to developers.

This situation was improved by providing the library identifier via a single field for all library formats (#1878). The name provided by this field is the "canonical" name of the library. Inexplicably, at that time the field containing the folder name was renamed CanonicalName. The string contained by this field is in no way a "canonical" name for the library, so the field name is bound to cause more of the very bugs and confusion the redesign of the struct was intended to prevent.

What is the new behavior?

Change the inappropriate github.com/arduino/arduino-cli/arduino/libraries.Library.CanonicalName field name to the accurate DirName.

Does this PR introduce a breaking change, and is titled accordingly?

Although the redesign of the github.com/arduino/arduino-cli/arduino/libraries.Library struct is a breaking change, this PR is only a patch on the previous PR that introduced the breaking change. Since there hasn't been a release since the time of that change, this PR does not introduce any new breaking changes.

…older name

Each Arduino library has a name, which is used as its sole identifier by the user in `arduino-cli lib` commands, and by
Arduino CLI when referring to the library in messages displayed to the user.

The name is defined by:

- "1.5 format" libraries: `name` field in the library.properties metadata file
- "1.0 format" (AKA "legacy") libraries: installation folder name

The name is resolved when loading the library and stored in the `Name` field of the
`github.com/arduino/arduino-cli/arduino/libraries.Library` struct.

The name of the library's installation folder is used by Arduino CLI in several other ways, most notably for determining
"folder name priority" for use in library dependency resolution. For this reason, the folder name is also stored in the
struct when loading the library.

Arduino CLI and arduino-builder have been plagued by problems caused by the inappropriate use of this folder name as the
identifier for the library instead of the sole correct identifier (which is only the folder name in the case of
"1.0 format" libraries. The design of the `github.com/arduino/arduino-cli/arduino/libraries.Library` struct may have
been a contributing factor in those bugs, since at the time of their occurrence the folder name was stored in the `Name`
field, the metadata-defined name in a `RealName`. In addition to the fact that no one field could be used as a source of
the name in all cases, I suspect the ambiguous field names themselves caused confusion to developers.

This situation was improved by providing the library identifier via a single field for all library formats. The name
provided by this field is the "canonical" name of the library. Inexplicably, at that time the field containing the
folder name was renamed "CanonicalName". The string contained by this field is in no way a "canonical" name for the
library, so the field name is bound to cause more of the same bugs and confusion the redesign of the struct was intended
to prevent.

The inappropriately named `github.com/arduino/arduino-cli/arduino/libraries.Library.CanonicalName` field is hereby
renamed to the accurate `DirName`.
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Oct 2, 2022
@per1234 per1234 requested a review from cmaglie October 2, 2022 08:01
@per1234 per1234 self-assigned this Oct 2, 2022
@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Base: 36.71% // Head: 36.76% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (cb7b0fb) compared to base (e404a3b).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
+ Coverage   36.71%   36.76%   +0.04%     
==========================================
  Files         231      231              
  Lines       19723    19723              
==========================================
+ Hits         7242     7251       +9     
+ Misses      11652    11645       -7     
+ Partials      829      827       -2     
Flag Coverage Δ
unit 36.76% <92.30%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/libraries/libraries.go 43.47% <ø> (ø)
legacy/builder/create_cmake_rule.go 8.80% <0.00%> (ø)
arduino/libraries/librariesresolver/cpp.go 91.27% <100.00%> (ø)
arduino/libraries/loader.go 70.45% <100.00%> (ø)
legacy/builder/phases/libraries_builder.go 32.45% <100.00%> (ø)
legacy/builder/types/types.go 62.50% <100.00%> (ø)
arduino/cores/packagemanager/package_manager.go 65.82% <0.00%> (-0.76%) ⬇️
arduino/monitor/monitor.go 47.36% <0.00%> (+6.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another meaning for "Canonical" in my mind, maybe because I'm not a native speaker.
BTW after reading your rationale, your changes make sense!

Thanks for taking the time to do this PR @per1234 !

@per1234 per1234 merged commit ce7b062 into arduino:master Oct 4, 2022
@per1234 per1234 deleted the library-folder-field-name branch October 4, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants